-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ToricVarieties] Tests for blowup do not depend on order of rays anymore #4353
Conversation
@test is_complete(amb1) == true | ||
@test is_simplicial(amb1) == true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@test is_complete(amb1) == true | |
@test is_simplicial(amb1) == true | |
@test is_complete(amb1) | |
@test is_simplicial(amb1) |
I am not a fan of all of these == true
here and further below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I removed those just now.
I would leave the question if that is mathematically sensible here to @lkastner and @benlorenz , since I do not really now whats going on here. |
9aeee2b
to
2a96fc9
Compare
Mathematically, the only significant change is that I am asking for the rank of a matrix instead of comparing it against an explicit hard-coded form of that matrix (which depends on the order of the rays). But then, fair. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4353 +/- ##
=======================================
Coverage 84.31% 84.31%
=======================================
Files 649 649
Lines 86343 86345 +2
=======================================
+ Hits 72797 72799 +2
Misses 13546 13546 |
In the log in #4348 there is a second failure which is probably related:
|
This PR aims to close #4348.
cc @lgoettgens @lkastner